chore(Android): Migrate Hermes Instruments to Kotlin #48378
Conversation
facebook-github-bot
added
CLA Signed
…utorFactory so it markes the NonNull values it overrides correctly.
| @@ -34,6 +35,7 @@ public void setDebuggerName(String debuggerName) { | |||
| mDebuggerName = debuggerName; | |||
| } | |||
|
|
|||
| @NonNull | |||
There was a problem hiding this comment.
JavaScriptExecutorFactory explicitly marks these values as non nullable, so it is ok to mark them here as well.
| String getInternalStorage(); | ||
| public fun getInternalStorage(): String | ||
|
|
||
| String getId(); | ||
| public fun getId(): String |
There was a problem hiding this comment.
I marked these as String and not String? since in Cxx the return type is std::string and not a pointer, but i do see some pointer shenanigans going on over there. Will leave it up to the person who reviews to give their thoughts as well. I can not find any use of this class anywhere, in Cxx or in the JVM land so i can't tell by usage
|
@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Hi @philIip. Can you please tell me what this change broke? Guessing the linter thing is because of character spacing but other than that i have no idea what this could have broken |
Hey @TheRogue76, there are some changes needed to let this code compile with the internal codebase, but those are unrelated to your changes. The PR on GitHub is good to go, and @philIip should be able to merge it in the coming days. Just note that due to holidays things were slower than usual so it might take a little longer till we're able to merge this one |
Much appreciated @cortinico . No rush on my side. |
|
This pull request was successfully merged by @TheRogue76 in d22dbb5 When will my fix make it into a release? | How to file a pick request? |
Summary:
Time to migrate some of Hermes's instruments. I see that HermesMemoryDumper(
react/hermes/instrumentation/HermesMemoryDumper.h) implements the interface on C++,but not sure if i need to update the
getIdcall to justid(same withgetInternalStorage) or if the interop between Kotlin and Java applies to these things as well. @cortinico Any thoughts on your side would be appreciated.HermesSamplingProfiler just became an object, since it was a singleton and a static anyway.
Here is what HermesMemoryDumper.h looks like:
Updated: I ended up making them match the function signature on Cxx, because even if it does have that implicit behavior, doesn't feel right to tap into it like this.
Changelog:
[INTERNAL] [FIXED] - Migrate HermesMemoryDumper and HermesSamplingProfiler to Kotlin
Test Plan:
/gradlew test: